Offline Directions errors-clarification #2374
Conversation
| return | ||
| } | ||
|
|
||
| guard result.isSuccess else { | ||
| DispatchQueue.main.async { | ||
| completionHandler(session, .failure(.noData)) |
There was a problem hiding this comment.
Seem like .noData is not the correct option here. result has only 2 properties exposed: isSuccess (which is so far was unused) and json (which may contain a valid response or an error message). In current implementation, I struggle to find a correct definition of "router returned no data" case to fire .noData message.
There was a problem hiding this comment.
This return value is a holdover from 0486778. It might be fine to reuse .noData for the .cancelled case. In the event of a failure, there should be an error that we can pass along.
| @@ -196,6 +203,13 @@ public class NavigationDirections: Directions { | |||
|
|
|||
| NavigationDirectionsConstants.offlineSerialQueue.async { [weak self] in | |||
| guard let result = self?.navigator.getRouteForDirectionsUri(url.absoluteString) else { | |||
There was a problem hiding this comment.
Per #2273 (comment), the only error case we should handle up here would be that self (NavigationDirections) has been deallocated.
Upon closer inspection, I’m wondering if it’s even appropriate to call the completion handler in this case. For mapbox/mapbox-gl-native-ios#210 mapbox/mapbox-gl-native-ios#200 (comment), we found that calling the completion handler after self has gone away forces the caller to consider whether self has gone away because the parent view controller has gone away or the application is terminating.
In other words, when an application calls this method from a MapViewController that holds a strong reference to NavigationDirections, does it expect the completion handler to magically be called even if that MapViewController has been deallocated (for example, because the user has exited that map screen to go to a different part of the application)? If the application has neglected to make self weak inside the completion handler, that could create other unexpected behavior in the application. The completion handler would have to explicitly handle the .cancelled case, so at least we would have to document that postcondition on the method, which is unfortunate.
There was a problem hiding this comment.
Not calling a completion under some conditions may also lead to unexpected behavior. At least such use case must also be documented so that users are aware that completion call is not guaranteed.
Also, if we choose not to rise an explicit error message when NavigationDirections were deallocated, noData error becomes unused, so we can safely remove it
| /** | ||
| The router did not finish the request | ||
| */ | ||
| case cancelled |
| @@ -34,6 +39,8 @@ public enum OfflineRoutingError: LocalizedError { | |||
| switch self { | |||
| case .standard(let error): | |||
| return error.localizedDescription | |||
| case .cancelled: | |||
| return NSLocalizedString("OFFLINE_CANCELLED", bundle: .mapboxCoreNavigation, value: "Routing was cancelled before response could be acquired.", comment: "Error description when Offline Router was deallocated before receiving the API response") | |||
There was a problem hiding this comment.
Here are some instructions for adding user-facing text to the SDK. The only remaining step would be to run scripts/extract_localizable.sh to add this string to a strings file.
| return | ||
| } | ||
|
|
||
| guard result.isSuccess else { | ||
| DispatchQueue.main.async { | ||
| completionHandler(session, .failure(.noData)) |
There was a problem hiding this comment.
This return value is a holdover from 0486778. It might be fine to reuse .noData for the .cancelled case. In the event of a failure, there should be an error that we can pass along.
…ect offline router object handling
…nstance was deallocated.
Udumft
force-pushed
the
2273-errors-clarification
branch
from
d897f7c to
7a83226
Compare
| @@ -174,7 +166,7 @@ public class NavigationDirections: Directions { | |||
|
|
|||
| - parameter options: A `RouteOptions` object specifying the requirements for the resulting routes. | |||
| - parameter offline: Determines whether to calculate the route offline or online. | |||
| - parameter completionHandler: The closure (block) to call with the resulting routes. This closure is executed on the application’s main thread. | |||
| - parameter completionHandler: The closure (block) to call with the resulting routes. This closure is executed on the application’s main thread. If called `NavigationDirections` instance is deallocated before route calculation is finished - completion won't be called. | |||
There was a problem hiding this comment.
Not familiar with context of this problem, so just minor comment from me: probably better to limit length of lines to around 100-120, just for better readability.
Resolves #2273
Added new error case for indicating incorrect offline router object handling.
I hope I got original idea, described in the linked issue correctly...
What is the correct way of adding new localizations here? How can I add
OFFLINE_CANCELLEDtranslation and verify that it's correct?